Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Added scripts to serve plugins locally for dev #140

Merged
merged 5 commits into from
Nov 30, 2023

Conversation

bmingles
Copy link
Contributor

  • Added an npm run serve script to serve plugins locally. This can be used with Vite proxy for DHC or DHE locally
  • Fixed missing nx native dependencies in package lock

@devinrsmith
Copy link
Member

Slightly related - would be reasonable for these use cases to disable js plugins on the server side: deephaven/deephaven-core#4695

Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Just a couple minor suggestions. We'll need a follow up PR in web-client-ui as well.
One thing this doesn't give us is source maps - but that will need a change from web-client-ui I think to achieve.

README.md Outdated Show resolved Hide resolved
vite.config.js Outdated
export default {
// This config doesn't build anything, it just serves the files from the
// plugins directory
publicDir: 'plugins',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like if we used port 4100 instead of the default port, and have the option to set it via environment variable/.env like in web-client-ui. Then we have:
3000: Enterprise
4000: web-client-ui
4010: Embedded grid
4020: Embedded chart
4100: JS Plugins

README.md Outdated
Comment on lines 109 to 115
```typescript
proxy['/js-plugins'] = {
target: 'http://localhost:5173',
changeOrigin: true,
rewrite: path => path.replace(/^\/js-plugins/, ''),
};
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a follow up PR to have a flag for this, something like JS_PLUGINS_PORT or something that only kicks the proxy in if the value is set.

Copy link
Contributor Author

@bmingles bmingles Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have another option for this that I've been playing with in my other PR.

https://github.com/deephaven/web-client-ui/pull/1661/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5

Can also see the value of having an env flag as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea a vite.config.local.ts could be useful for some things as well... annoying to add that extra flag for it, but I guess either way we're adding an extra flag somewhere

@mofojed
Copy link
Member

mofojed commented Nov 30, 2023

Also rebase off the latest so tests run correctly

@bmingles bmingles force-pushed the 1504-plugin-dev-server branch from 6d31b7a to 7338ae4 Compare November 30, 2023 14:48
@bmingles bmingles requested a review from mofojed November 30, 2023 15:09
@bmingles bmingles merged commit 1234151 into deephaven:main Nov 30, 2023
8 checks passed
@bmingles bmingles deleted the 1504-plugin-dev-server branch November 30, 2023 16:20
npm run serve
```

This will serve the `plugins` directory at `http://localhost:5173`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the port in the readme match the default in the config?

const DEFAULT_PORT = 4100;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'll fix in another PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants